-
-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add colordict and sysfont to typing, move PowerState class to typing module #3133
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can PowerState
be private if it's not prefixed with an underscore? (not documenting it isn't really enough when it comes to python files)
c8afa8d
to
54d278d
Compare
When I say private, I mean that it's not a part of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright then, LGTM :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if this is more accurate I don't want to leak the internal implementation details into our stable "type API"
For example, we could reduce memory usage or do interesting things with THECOLORS, right? Like what if we had it as a str to one integer (RRGGBBAA) instead of (R, G, B, A). We could go from the overhead of 4 python integers and a tuple down to 1 integer, which would be a bit less memory. No, we can't mess with it because it always has to be a dictionary of str to RGBA, and is exposed as public API by the type system.
(This is something I ran into the other day, before or after your PR the THECOLORS has already been leaked. I'm using it as an example of over-typing restricting our freedom to change things)
With that in mind I don't see why we would expose the existence of the colordict and sysfont modules, given they are 100% implementation details.
I'm also somewhat wary of moving an actual full declaration of one our classes into typing.py, that seems strange.
Well, if they were intended to being implementation details but got leaked at runtime, they are not any more. A quick github search reveals pygame.sysfont is also "auto-imported" at runtime. As expected, a quick github code search will show that this is also used in user code. Even though at the moment it is explicitly ignored in the stubs. My conclusion? Hiding or not hiding something in the stubs does not change the fact that implementation behaviour that is exposed eventually becomes API. And because we are now not at the liberty to remove it, we might as well roll with it and make it solid |
It seemed weird to have a singleton module just for defining a class. I feel like it logically belongs to |
I disagree. Yes, |
Well, even if we have to keep it as it is, I would still like to give it the |
Would it be possible to type |
That is also doable, but then we have to maintain the duplication of the definition in the source and the stub manually. |
I don't think sysfont is supposed to be auto-imported. I think the authors of this primordial code imported it so the could put functions defined in that file onto the font module, and then they neglected the "del sysfont" step to remove it from the namespace. Just because an implementation detail is leaked doesn't mean it should be treated as an API. Where are the docs for the sysfont module? Where are the examples of use? They aren't there and they shouldn't be there. There are tests but that makes sense as as a test of a system. As far as I'm concerned in pygame-ce 3 we should rename sysfont to Similarly, colordict is not a module, it's an implementation detail. That's why if you go to the top of the docs site there isn't a place called "colordict".
Ok fine it's a module at runtime but there's no reason it should or has to be, and I don't think we should treat it how we treat something like |
And in terms of powerstate can the pyi file import a definition from a py file? |
It can, but sometimes it doesn't and then mypy complains about it. I don't know how or why it happens though |
If we had a pure Python module would we need type stubs for it at all? What if pygame.system was a python module that called into C code to provide its functionality? |
Technically no, but so far we have had stubs for the python modules as well. One of the things here that I think is causing mypy analysis issues is that the stubs and python sources are in different folders when they should be in the same folder (and after installation, they actually are) |
colordict
andsysfont
are actually public modules, so they are now typed.PowerState
class has now been moved totyping.py
, so that it can be resolved at the stub level correctly. But it is a private member oftyping.py
.